Skip to content

More pytest optimizations - #3868

Merged
cdecker merged 10 commits into
ElementsProject:masterfrom
rustyrussell:guilt/slow-tests-marked
Aug 7, 2020
Merged

More pytest optimizations#3868
cdecker merged 10 commits into
ElementsProject:masterfrom
rustyrussell:guilt/slow-tests-marked

Conversation

@rustyrussell

@rustyrussell rustyrussell commented Jul 23, 2020

Copy link
Copy Markdown
Contributor
  1. A flake fix for test_restart_many_payments.
  2. A flake fix for test_onchain_their_unilateral_out
  3. Mass conversion to get_nodes() which batches, vs. individual get_node() calls.
  4. Turn off valgrind for SLOW_MACHINE slow tests, rather than skipping them entirely.

Changelog-None

@rustyrussell rustyrussell added this to the Next Release milestone Jul 23, 2020
@rustyrussell
rustyrussell requested a review from cdecker as a code owner July 23, 2020 07:18
@rustyrussell
rustyrussell force-pushed the guilt/slow-tests-marked branch from c28d9b5 to a5b8034 Compare July 24, 2020 00:38
@rustyrussell
rustyrussell force-pushed the guilt/slow-tests-marked branch from a5b8034 to e5dd729 Compare August 3, 2020 02:43
@rustyrussell

Copy link
Copy Markdown
Contributor Author

Trivial rebase

I thought this was timing out because I made it slow with the
change to txprepare as a plugin.  In fact, it was timing out
because sometimes gossip comes so fast it gets suppressed
and we never get the log messags.

Still, before this it took 98 seconds under valgrind and
24 under non-valgrind, so it's an improvement to time as
well as robustness.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…me of them.

This lets you get_nodes() and join some later.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is what fund_channel() does, which is more thorough than what
we were doing.  But since the order of the logs is undefined, we need
to be a little careful.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In fact, the 5 blocks generate above were not seen by nodes until
later, making me very confused.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
rustyrussell force-pushed the guilt/slow-tests-marked branch from e5dd729 to be21b6b Compare August 4, 2020 20:51
@rustyrussell

Copy link
Copy Markdown
Contributor Author

Trivial rebase, and fix to actually turn off VALGRIND on slow tests!

@niftynei niftynei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK be21b6b

Comment thread tests/test_pay.py Outdated

scid12 = l1.get_channel_scid(l2)
scid23 = l2.get_channel_scid(l3)
print(l1.rpc.getroute(l2.info['id'], 10000, 1))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not necessary?

Comment thread tests/test_plugin.py
Comment thread contrib/pyln-testing/pyln/testing/utils.py
We can have the message before the node ready message which line_graph
waits for.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We might have funds prior to calling join_nodes(), so testing that
we've all seen the block is better.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I started replacing all get_node() calls, but got bored, so then just did the
tests which call get_node() 3 times or more.

Ends up not making a measurable speed difference, but it does make some
things neater and more standard.

Times with SLOW_MACHINE=1 (given that's how Travis tests):

Time before (non-valgrind):
	393 sec (had 3 failures?)
Time after (non-valgrind):
	410 sec

Time before (valgrind):
	890 seconds (had 2 failures)
Time after (valgrind):
	892 sec

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can query all the txids at once, rather than one at a time.

Doesn't make any measurable difference to full runtime testing here
though.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Next patch will turn it off for slow-marked tests.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And when it's set, and we're SLOW_MACHINE, simply disable valgrind.

Since Travis (SLOW_MACHINE=1) only does VALGRIND=1 DEVELOPER=1 tests,
and VALGRIND=0 DEVELOPER=0 tests, it was missing tests which needed
DEVELOPER and !VALGRIND.

Instead, this demotes them to non-valgrind tests for SLOW_MACHINEs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
rustyrussell force-pushed the guilt/slow-tests-marked branch from be21b6b to 6c9d196 Compare August 7, 2020 03:15
@cdecker

cdecker commented Aug 7, 2020

Copy link
Copy Markdown
Member

ACK 6c9d196

@cdecker
cdecker merged commit 01a82d3 into ElementsProject:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants